Modify NSModel to use boost variant. #693

MarcosPividori · 2016-06-13T14:19:40Z

No description provided.

rcurtin · 2016-06-13T19:54:48Z

src/mlpack/methods/neighbor_search/ns_model.hpp

@@ -13,12 +13,24 @@
 #include <mlpack/core/tree/binary_space_tree.hpp>
 #include <mlpack/core/tree/cover_tree.hpp>
 #include <mlpack/core/tree/rectangle_tree.hpp>
-
+#include <boost/variant.hpp>


Should we add 'variant' to the list of Boost libraries we require in the documentation and in the root CMakeLists.txt?

It is a header only library, so we don't need to include it in CMakeList.txt.
I can see that all boost dependencies mentioned in the README file: program_options, math_c99, unit_test_framework, and serialization, are not header only. Should I include variant there anyway? even if it is header only?

You're right, it is header only. I am not sure if it is possible to have a Boost installation where some of the header-only libraries (but not all) are available; I haven't played with b2 (Boost's build system) enough recently to be sure. If it's possible to have a Boost installation with some of the header-only libraries omitted, then we should include variant in there; otherwise, I don't think it's necessary. But I'm not sure whether or not that's possible...

After a quick search, it looks like there is no such option to omit some header files...

Ok, looks like there is nothing special we need to do with our CMake configuration for this change then.

but we don't have to add boost::variant to required libraries?? Even if its header only we need to make sure the headers are present. Or is it the part of basic boost which is included in every boost package?? I mean if I install just boost::math in ubuntu with apt-get do I get boost::variant??

I can see a note about this in boost's documentation: [ref]
"Other Packages
RedHat, Debian, and other distribution packagers supply Boost library packages, however you may need to adapt these instructions if you use third-party packages, because their creators usually choose to break Boost up into several packages, reorganize the directory structure of the Boost distribution, and/or rename the library binaries. If you have any trouble, we suggest using an official Boost distribution from SourceForge. "

I am not sure. But it looks like all partial packages depends on libboost-dev package, so they will finally include all headers files.
For example: libboost-program-options-dev --depends on--> libbooost-dev

MarcosPividori · 2016-06-13T20:02:48Z

The problem with appveyor is similar to this one: #476
With "using NSType = ...".

 error C3200: 'BinarySpaceTree<MetricType,mlpack::neighbor::NeighborSearchStat<Unique-Type-SortPolicy>,
 MatType,mlpack::bound::HRectBound,mlpack::tree::MidpointSplit>::DualTreeTraverser':
 invalid template argument for template parameter 'TraversalType', expected a class template

@rcurtin @stereomatchingkiss Any suggestion?
It is a bit difficult to debug from my linux computer.

rcurtin · 2016-06-13T20:04:30Z

src/mlpack/methods/neighbor_search/ns_model.hpp

+                  arma::mat& distances);
+};
+
+class SearchVisitor : public boost::static_visitor<void>


Maybe we should change the name of this class and SearchKVisitor to BichromaticSearchVisitor and MonochromaticSearchVisitor, respectively, for clarity? I couldn't think of any shorter names to differentiate there, but maybe you have a better idea? The root of my concern is that Search and SearchK don't really describe the difference in what is going on.

Ok, I agree. I will change this.

stereomatchingkiss · 2016-06-15T06:51:23Z

Hi, Marcos, I found a way to mimic alias template, please check the codes at pastebin.

…rainVisitor. Also, add a more specific definition of NSTypeT, to avoid vc compiler errors.

MarcosPividori · 2016-06-15T14:47:48Z

@stereomatchingkiss thanks!! I am considering that option.

MarcosPividori · 2016-06-15T16:44:45Z

@stereomatchingkiss AppVeyor succeeded! I have made it a little different to avoid code bloat.

MarcosPividori · 2016-06-16T03:39:01Z

I think this is ready to be merged. Please let me know if you want me to improve something.

rcurtin · 2016-06-16T23:36:26Z

src/mlpack/methods/neighbor_search/ns_model.hpp

@@ -37,6 +49,127 @@ struct NSModelName<FurthestNeighborSort>
  static const std::string Name() { return "furthest_neighbor_search_model"; }
 };

+class MonoSearchVisitor : public boost::static_visitor<void>


Ah, I hate to be picky, so I hope I am not giving too much work, but do you think you could add documentation for these classes and their members and member functions? I am not sure everyone who looks through this file will be familiar with the visitor paradigm; there is no need to explain what that is in your comments, but it may be useful to have comments along the lines of "MonoSearchVisitor executes a monochromatic neighbor search on the given NSType", or something like this. Maybe a few more words are useful to explain that better. :)

Actually, I should add, if you don't want to or are busy doing other things, I can do the documentation here, it will only take a few minutes.

@rcurtin Sure!! I am here for this! I agree it can be confusing for someone with no knowledge of boost variants. I will add documentation.
Thanks!

Done in: 6c2c3ca

MarcosPividori · 2016-06-17T17:03:12Z

Hi, in b34dac8 , I fixed serialization of NSModel. Loading was not working before that commit.
I use the serialize function for variants, defined by boost. So, I had to add a definition of serialize for NeighborSearchClass, because it is need by boost variant serialization.
Another option to fixing the serialization can be seeing in this branch: https://github.com/MarcosPividori/mlpack/tree/variant-serialization , but I prefer the solution included above, because it is simpler.

rcurtin · 2016-06-17T17:05:54Z

Everything looks good to me; @sumedhghaisas, I have no more comments, so feel free to merge when you are happy. :)

MarcosPividori mentioned this pull request Jun 13, 2016

Remove code redundancy in ns_model_impl.hpp #674

Closed

MarcosPividori force-pushed the variants-for-ns-model branch from 0429c95 to 5290e37 Compare June 13, 2016 14:58

Modify NSModel to use boost variant.

86a9852

MarcosPividori force-pushed the variants-for-ns-model branch from 5290e37 to 86a9852 Compare June 13, 2016 18:29

rcurtin reviewed Jun 13, 2016
View reviewed changes

Improve name of Search visitors.

a0bcee9

MarcosPividori force-pushed the variants-for-ns-model branch 2 times, most recently from 1b87667 to a0bcee9 Compare June 15, 2016 02:55

Set SortPolicy as template parameter of classes BiSearchVisitor and T…

ddb252c

…rainVisitor. Also, add a more specific definition of NSTypeT, to avoid vc compiler errors.

Details to improve code and documentation.

83f1925

rcurtin reviewed Jun 16, 2016
View reviewed changes

MarcosPividori added 2 commits June 16, 2016 23:55

Improve documentation.

6c2c3ca

Fix NSModel serialization. Use boost serialize function for variants.

b34dac8

MarcosPividori force-pushed the variants-for-ns-model branch from 931e753 to b34dac8 Compare June 17, 2016 07:23

sumedhghaisas merged commit 779556f into mlpack:master Jun 20, 2016

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Modify NSModel to use boost variant. #693

Modify NSModel to use boost variant. #693

MarcosPividori commented Jun 13, 2016

rcurtin Jun 13, 2016

MarcosPividori Jun 13, 2016

rcurtin Jun 13, 2016

MarcosPividori Jun 13, 2016

rcurtin Jun 14, 2016

sumedhghaisas Jun 15, 2016

MarcosPividori Jun 15, 2016

MarcosPividori commented Jun 13, 2016 •

edited

rcurtin Jun 13, 2016

MarcosPividori Jun 13, 2016

stereomatchingkiss commented Jun 15, 2016

MarcosPividori commented Jun 15, 2016

MarcosPividori commented Jun 15, 2016

MarcosPividori commented Jun 16, 2016

rcurtin Jun 16, 2016

rcurtin Jun 16, 2016

MarcosPividori Jun 17, 2016 •

edited

MarcosPividori Jun 17, 2016

MarcosPividori commented Jun 17, 2016 •

edited

rcurtin commented Jun 17, 2016

Modify NSModel to use boost variant. #693

Modify NSModel to use boost variant. #693

Conversation

MarcosPividori commented Jun 13, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

MarcosPividori commented Jun 13, 2016 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

stereomatchingkiss commented Jun 15, 2016

MarcosPividori commented Jun 15, 2016

MarcosPividori commented Jun 15, 2016

MarcosPividori commented Jun 16, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

MarcosPividori Jun 17, 2016 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

MarcosPividori commented Jun 17, 2016 • edited

rcurtin commented Jun 17, 2016

MarcosPividori commented Jun 13, 2016 •

edited

MarcosPividori Jun 17, 2016 •

edited

MarcosPividori commented Jun 17, 2016 •

edited